Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move default pagination to 20, 50, 100, all #1400

Merged
merged 10 commits into from
Mar 11, 2024

Conversation

lguichard
Copy link
Contributor

Currently, the default pagination use Filament default configuration. I purpose to change this

It is more ergonomic and useful, show the products and items in a trice

Copy link

vercel bot commented Dec 20, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lunar-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 11, 2024 8:41pm

@glennjacobs
Copy link
Contributor

This does raise the question as to whether we should have "all" as a default option.

Some tables will have thousands of records and so "all" is likely to be quite dangerous.

@glennjacobs glennjacobs added the enhancement Improvement to existing functionality label Dec 20, 2023
@glennjacobs glennjacobs added this to the v1.0 milestone Dec 20, 2023
@adevade
Copy link
Contributor

adevade commented Dec 20, 2023

This does raise the question as to whether we should have "all" as a default option.

Some tables will have thousands of records and so "all" is likely to be quite dangerous.

I usually remove it from my Filament tables for that reason, so +1 from me.

@lguichard
Copy link
Contributor Author

@glennjacobs correct, i will remove 'all' option. During my tests, 100 lines was the maximum for having a quick load.

@binaryfire
Copy link

binaryfire commented Dec 20, 2023

There's an open Filament issue re: large table performance: filamentphp/filament#9304. The consensus there was 100 max as well.

@glennjacobs I remember you saying something about wrangling with Livewire performance when creating Lunar’s current datatables package. Maybe you've got some insights you could share in the Filament issue to help the team improve this?

@glennjacobs
Copy link
Contributor

There's an open Filament issue re: large table performance: filamentphp/filament#9304. The consensus there was 100 max as well.

@glennjacobs I remember you saying something about wrangling with Livewire performance when creating Lunar’s current datatables package. Maybe you've got some insights you could share in the Filament issue to help the team improve this?

The performance there was mainly Eloquent related which Filament v3 seems to have addressed pretty well. I agree, 100 max.

@wychoong
Copy link
Contributor

can this be configurable or use extension

@lguichard
Copy link
Contributor Author

@wychoong Exactly, i can create config/panel.php with configuration variables of panel and move pagination default.
@alecritson Do you have planned separate issue with global configuration of panel ?

@glennjacobs
Copy link
Contributor

I'm not keen on the config approach initially.

Having looked at Filament there doesn't appear to be a way to set the options globally.

I do wonder if Filament would consider a PR to allow pagination values to be set globally on the panel or something. I'd need to look at how doable that would be. Then it would be easy for a developer to change.

@adevade
Copy link
Contributor

adevade commented Dec 22, 2023

@glennjacobs
Copy link
Contributor

You mean setting global pagination settings like this?

Ahha! I was searching for that and couldn't find it... that's exactly it.

So we could look to use that in our service provider, as long as devs can override it in their app service provider.

@lguichard
Copy link
Contributor Author

Nice thanks @sandervankasteel

Vous voulez dire définir des paramètres de pagination globale comme celui-ci  ?

We have two solutions to implement the global settings table configuration :

First : we create a new options on LunarPanel Facade

      LunarPanel::configureTable(fn(Table $table) : void {
           // ...
        });

Second : Add on LunarPanelManger

        /*
        *   Default global settings for Page
        */
        Table::configureUsing(function (Table $table): void {
            $table
                ->paginationPageOptions([20, 50, 100]);
        });

For custom by devs :

    // AppServiceProvider.php 
    // ...
    public function register(): void
    {
        // Register panel
        LunarPanel::register();

        // Override panel
        LunarPanel::panel(fn($panel) =>
            $panel->path('lunar')
                ->brandName('My custom name')
        )
        ->register();

        // Override default configuration
        Table::configureUsing(function (Table $table): void {
            $table
                ->paginationPageOptions([1, 5, 2]);
        });
       // ...

What do we do ? @glennjacobs

@glennjacobs
Copy link
Contributor

I would put the following in the Lunar service provider

Table::configureUsing(function (Table $table): void {
    $table
        ->paginationPageOptions([20, 50, 100])
        >defaultPaginationPageOption(20);
});

and then test that a developer can override this is their own app service provider.

@binaryfire
Copy link

binaryfire commented Dec 22, 2023

@glennjacobs This will make all Filament tables (including non-Lunar ones) use these pagination options.

@glennjacobs
Copy link
Contributor

@glennjacobs This will make all Filament tables (including non-Lunar ones) to use these pagination options.

Ah yes, that's a good point.

@lguichard
Copy link
Contributor Author

I would put the following in the Lunar service provider

Table::configureUsing(function (Table $table): void {
    $table
        ->paginationPageOptions([20, 50, 100])
        >defaultPaginationPageOption(20);
});

and then test that a developer can override this is their own app service provider.

That's it, i test override on AppServiceProvider works well

@wychoong
Copy link
Contributor

Use the resource extension, and a global in lunar panel manager. So that a global can be set there, and control for each resources

@glennjacobs glennjacobs merged commit 6240b3c into lunarphp:1.x Mar 11, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement to existing functionality
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants